Add new AvoidSecretDisclosure rule by iRon7 · Pull Request #2183 · PowerShell/PSScriptAnalyzer

iRon7 · 2026-05-06T09:05:55Z

PR Summary

Closes: #1997

Description

Disclosing a secret might result in security vulnerabilities such as memory trails or logging trails that could
be exploited by attackers. This rule identifies instances where a secret is being converted to plain text,
which can lead to unintended exposure of sensitive information.

Important

The general approach of dealing with credentials is to avoid them and instead rely on other means
to authenticate, such as certificates or Windows authentication.

How to Fix

In general, avoid any code pattern that involves converting secrets to plaintext or accessing plaintext secrets.

For ConvertFrom-SecureString -AsPlainText: Use -Credential parameter instead
For SecureStringTo* methods: Avoid converting to plaintext
For Password properties: Use secure credential objects directly or the SecureString equivalent
SecurePassword instead of accessing plaintext passwords.

Note

For custom properties named "Password", it is recommended to rename them to something that does not imply they
contain secrets, or to ensure that they do not actually contain secrets. If renaming is not possible, consider
suppressing the warning for those specific cases.

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Copilot

Pull request overview

Adds a new built-in PSScriptAnalyzer rule (PSAvoidSecretDisclosure) to flag common patterns that convert secrets (e.g., SecureString) into plaintext, with accompanying tests, localized strings, and documentation.

Changes:

Introduces AvoidSecretDisclosure rule implementation to detect ConvertFrom-SecureString -AsPlainText, SecureStringTo* calls, and .Password member access.
Adds Pester tests covering violations, compliant examples, and suppression scenarios.
Updates rule documentation and the rules index, and adds localized strings for name/common name/description/error.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Tests/Rules/AvoidSecretDisclosure.tests.ps1	Adds Pester coverage for the new rule (violations/compliance/suppression).
Rules/Strings.resx	Adds localized strings for the new rule’s name/common name/description/message.
Rules/AvoidSecretDisclosure.cs	Implements the new analyzer rule logic and diagnostic creation.
docs/Rules/README.md	Registers the rule in the published rules list/table.
docs/Rules/AvoidSecretDisclosure.md	Adds the rule’s public documentation page and examples.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sean Wheeler (sdwheeler)

Added suggestions for style and one question about the parameter.

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

…ble in a future release after giving users time to adjust.

Christoph Bergmeister (bergmeister)

There is already a AvoidUsingConvertToSecureStringWithPlainText therefore if we were ti have a general rule to alert on things like SecureString methods, then the old rule logic should be merged into this one and there should be configuration on what to alert for. For example on Linux, Credential does not apply. And on Linux SecureString is not implemented and since .NET 5, Securestring was marked as obsolete and not recommended any more as it is understood that practically it is and was never more secure in first place.
Since it's an optional rule, we can be opinionated but in practice many warnings will not be fixable and we found that this can be a source of frustration for community.

iRon7 · 2026-05-08T08:54:06Z

Christoph Bergmeister (@bergmeister),

The difference with the existing AvoidUsingConvertToSecureStringWithPlainText rule is that this rule warns for a convert from plain text in contrast to this new AvoidSecretDisclosure rule which warns for converting a secret to plain text.
Imho, for the first rule there are a few exceptions to put something in a SecureString (e.g. Read-Host -AsSecureString), for the new AvoidSecretDisclosure rule, any action that requires disclosing a secret (SecureString) in a script is questionable from a security standpoint.

Since it's an optional rule, we can be opinionated but in practice many warnings will not be fixable and we found that this can be a source of frustration for community.

Agree, this rule should be disabled by default for continuous integration usage and enabled manually as it will certainly reveal a lot of security violations in the currently installed bases...

…ext`

Sean Wheeler (sdwheeler)

Docs look good.

iRon7 · 2026-06-10T15:25:26Z

Since about 3 weeks ago, I have created 6 PR for new rules (the last one was from yesterday: #2186).
From the first 5, 4 PRs have been merged in the main branch by Andy Jordan (@andyleejordan) except for this one.
I am not sure what the reason is that this one is stalled, but I guess that we are waiting for Christoph Christoph Bergmeister (@bergmeister) to react and taking my arguments on his earlier comments in consideration.
Anyways, let me know if there is anything expected from my side to further process this one.

Copilot

🟡 Changes recommended

The rule currently misses valid disclosure syntax, reports a secure PSCredential.Password access, and provides misleading remediation guidance.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Rules/AvoidSecretDisclosure.cs:76

A quoted member name such as [Marshal]::'SecureStringToBSTR'(...) invokes the same conversion, but matching the member's source text can retain the quotes and fail this prefix check. Match StringConstantExpressionAst.Value instead and use that normalized value for the suppression ID.

                testAst is InvokeMemberExpressionAst invokeAst &&
                invokeAst.Member != null &&
                invokeAst.Member.ToString().StartsWith("SecureStringTo", StringComparison.OrdinalIgnoreCase),

Rules/AvoidSecretDisclosure.cs:94

This name-only check also warns on [PSCredential].Password, whose value is already a SecureString; reading it does not disclose plaintext and is a valid input to APIs that accept SecureString. Exclude properties that can be resolved to SecureString (at least statically typed PSCredential instances), while retaining the heuristic only where the property type is unknown or plaintext.

            IEnumerable<MemberExpressionAst> passwordAsts = ast.FindAll(testAst =>
                testAst is MemberExpressionAst memberAst &&
                memberAst.Member != null &&
                string.Equals(memberAst.Member.ToString(), "Password", StringComparison.OrdinalIgnoreCase),

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

Christoph Bergmeister (bergmeister) · 2026-09-14T20:10:36Z

+                cmdAst.GetCommandName() != null &&
+                cmdAst.GetCommandName().Equals("ConvertFrom-SecureString", StringComparison.OrdinalIgnoreCase),


whilst it would be good to also cater for module qualified, current approach is good enough IMHO

Christoph Bergmeister (bergmeister) · 2026-09-14T20:09:46Z

+In general, avoid any code pattern that involves converting secrets to plaintext or accessing
+plaintext secrets.
+
+- For `ConvertFrom-SecureString -AsPlainText`: Use `-Credential` parameter instead


iRon7 maybe I am reading this with not enough context but what do you mean by -Credential parameter?

Christoph Bergmeister (bergmeister)

happy with it, first copilot suggestion I see as optional improvement. Just a reply please on last one to clarify what you mean with -Credential param

Add new AvoidSecretDisclosure rule

586302f

Copilot AI review requested due to automatic review settings May 6, 2026 09:05

iRon7 requested review from a team, Christoph Bergmeister (bergmeister), Mikey Lombardi (He/Him) (michaeltlombardi) and Sean Wheeler (sdwheeler) as code owners May 6, 2026 09:05

Copilot started reviewing on behalf of iRon7 May 6, 2026 09:06 View session

Merge branch 'main' into AvoidSecureStringDisclosure

9b67e56

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread Rules/AvoidSecretDisclosure.cs

Comment thread Rules/AvoidSecretDisclosure.cs Outdated

Comment thread docs/Rules/AvoidSecretDisclosure.md

Comment thread Tests/Rules/AvoidSecretDisclosure.tests.ps1

Implemented GitHub Copilot suggestions

014cfba

iRon7 requested a review from Copilot May 6, 2026 10:48

Copilot started reviewing on behalf of iRon7 May 6, 2026 10:49 View session

Copilot AI reviewed May 6, 2026

iRon7 mentioned this pull request May 6, 2026

Add new AvoidDynamicallyCreatingVariableNames rule #2178

Merged

6 tasks

Sean Wheeler (sdwheeler) requested changes May 6, 2026

View reviewed changes

Comment thread docs/Rules/AvoidSecretDisclosure.md Outdated

Comment thread docs/Rules/AvoidSecretDisclosure.md

Comment thread docs/Rules/AvoidSecretDisclosure.md Outdated

Comment thread docs/Rules/AvoidSecretDisclosure.md Outdated

Comment thread docs/Rules/AvoidSecretDisclosure.md Outdated

iRon7 and others added 6 commits May 7, 2026 15:51

Update docs/Rules/AvoidSecretDisclosure.md

b2ed757

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

Update docs/Rules/AvoidSecretDisclosure.md

83b4194

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

Update docs/Rules/AvoidSecretDisclosure.md

45813ad

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

Update docs/Rules/AvoidSecretDisclosure.md

3473048

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

Update docs/Rules/AvoidSecretDisclosure.md

bc94e77

Co-authored-by: Sean Wheeler <sean.wheeler@microsoft.com>

Disabled new rule by convention, as it is a breaking change. Will ena…

d62673c

…ble in a future release after giving users time to adjust.

Christoph Bergmeister (bergmeister) reviewed May 7, 2026

View reviewed changes

Windows PowerShell doesn't have a `ConvertFrom-SecureString -AsPlainT…

635c041

…ext`

Sean Wheeler (sdwheeler) approved these changes May 15, 2026

View reviewed changes

Merge branch 'main' into AvoidSecureStringDisclosure

fc60573

Merge branch 'main' into AvoidSecureStringDisclosure

31f0bd3

Christoph Bergmeister (bergmeister) requested a balanced review from Copilot September 14, 2026 19:58

Copilot started reviewing on behalf of Christoph Bergmeister (bergmeister) September 14, 2026 19:59 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Merge branch 'main' into AvoidSecureStringDisclosure

5d8b9e0

Christoph Bergmeister (bergmeister) approved these changes Sep 14, 2026

View reviewed changes

		cmdAst.GetCommandName() != null &&
		cmdAst.GetCommandName().Equals("ConvertFrom-SecureString", StringComparison.OrdinalIgnoreCase),

Uh oh!

Conversation

iRon7 commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Description

How to Fix

PR Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Sean Wheeler (sdwheeler) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Christoph Bergmeister (bergmeister) left a comment

Choose a reason for hiding this comment

Uh oh!

iRon7 commented May 8, 2026

Uh oh!

Sean Wheeler (sdwheeler) left a comment

Choose a reason for hiding this comment

Uh oh!

iRon7 commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Christoph Bergmeister (bergmeister) Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Christoph Bergmeister (bergmeister) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iRon7 commented May 6, 2026 •

edited

Loading